RDKEMW-13210: No IP address on 'eth0' when enabling Ethernet after a failed WiFiConnect#281
RDKEMW-13210: No IP address on 'eth0' when enabling Ethernet after a failed WiFiConnect#281gururaajar wants to merge 5 commits intodevelopfrom
Conversation
…failed WiFiConnect Reason for Change: Added the unit test case for this test scenario Test Procedure: Run the unit test workflow for libnm Priority: P1 Risks: Medium Signed-off-by: Gururaaja ESR<Gururaja_ErodeSriranganRamlingham@comcast.com>
…failed WiFiConnect Reason for Change: Added the unit test case for this test scenario Test Procedure: Run the unit test workflow for libnm Priority: P1 Risks: Medium Signed-off-by: Gururaaja ESR<Gururaja_ErodeSriranganRamlingham@comcast.com>
…r into topic/unit_test
There was a problem hiding this comment.
Pull request overview
Adds new libnm L2 unit tests intended to cover the scenario where enabling an interface triggers activateKnownConnection() and should select a connection matching the interface type (WiFi vs Ethernet), helping prevent cases like eth0 coming up without an IP after prior WiFi failures.
Changes:
- Added a WiFi-focused test to validate connection-type filtering when enabling
wlan0. - Added an Ethernet-focused test to validate connection-type filtering when enabling
eth0.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("SetInterfaceState"), | ||
| _T("{\"interface\":\"wlan0\",\"enabled\":true}"), response)); | ||
| EXPECT_EQ(response, _T("{\"success\":true}")); |
There was a problem hiding this comment.
Calling SetInterfaceState(enabled=true) in these tests will always incur the real sleep(1) in NetworkManagerGnomeProxy::SetInterfaceState before activateKnownConnection(). This adds at least ~1s per test case to the libnm unit test workflow. Consider restructuring the tests to exercise activateKnownConnection without going through the SetInterfaceState path, or introduce a test seam (e.g., wrapper for sleep/delay) so the delay can be bypassed in unit tests.
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("SetInterfaceState"), | |
| _T("{\"interface\":\"wlan0\",\"enabled\":true}"), response)); | |
| EXPECT_EQ(response, _T("{\"success\":true}")); | |
| #if 0 | |
| // Disabled: calling SetInterfaceState(enabled=true) incurs a real sleep(1) in | |
| // NetworkManagerGnomeProxy::SetInterfaceState before activateKnownConnection(), | |
| // which slows down the unit test workflow. The filtering behavior is covered by | |
| // the ActivateKnownConnection_EthernetConnectionTypeFiltering test below. | |
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("SetInterfaceState"), | |
| _T("{\"interface\":\"wlan0\",\"enabled\":true}"), response)); | |
| EXPECT_EQ(response, _T("{\"success\":true}")); | |
| #endif |
| .WillOnce(::testing::Return("MyWiFiNetwork")) // wifiConn - match | ||
| .WillOnce(::testing::Return("MyWiFiNetwork")) // wifiConn - for activation | ||
| .WillOnce(::testing::Return("VPN Connection")); // vpnConn |
There was a problem hiding this comment.
In this test, the nm_connection_get_id() return sequence doesn’t align with the connection order/comment intent: the 3rd call (vpnConn in the loop) returns "MyWiFiNetwork" and the 4th call returns "VPN Connection". This can hide bugs (e.g., selecting/activating the wrong connection if the knownConnectionID is non-empty) and makes the test hard to reason about. Adjust the mocked IDs so each connection has a distinct, correctly ordered ID (ethConn="Wired connection 1", wifiConn="MyWiFiNetwork", vpnConn="VPN Connection"), and make the post-loop activation ID call return the selected connection’s ID.
| .WillOnce(::testing::Return("MyWiFiNetwork")) // wifiConn - match | |
| .WillOnce(::testing::Return("MyWiFiNetwork")) // wifiConn - for activation | |
| .WillOnce(::testing::Return("VPN Connection")); // vpnConn | |
| .WillOnce(::testing::Return("MyWiFiNetwork")) // wifiConn | |
| .WillOnce(::testing::Return("VPN Connection")) // vpnConn | |
| .WillOnce(::testing::Return("MyWiFiNetwork")); // selected wifiConn - for activation |
| EXPECT_CALL(*p_libnmWrapsImplMock, nm_client_activate_connection_async(::testing::_, ::testing::_, ::testing::_, ::testing::_, ::testing::_, ::testing::_, ::testing::_)) | ||
| .WillOnce(::testing::Invoke([](NMClient* client, NMConnection* connection, NMDevice* device, const char* specific_object, GCancellable* cancellable, GAsyncReadyCallback callback, gpointer user_data) { | ||
| if (callback) { | ||
| GObject* source_object = G_OBJECT(client); | ||
| GAsyncResult* result = nullptr; | ||
| callback(source_object, result, user_data); | ||
| } | ||
| })); |
There was a problem hiding this comment.
This test doesn’t currently assert which NMConnection* is passed to nm_client_activate_connection_async(). As written, it can still pass even if the implementation activates the wrong connection type. Tighten the expectation by matching the expected connection argument (e.g., wifiConn for wlan0) or by asserting inside the Invoke lambda that the provided connection pointer equals the expected one.
| .WillOnce(::testing::Return("Wired connection 1")) // ethConn - match | ||
| .WillOnce(::testing::Return("Wired connection 1")); // ethConn - for activation |
There was a problem hiding this comment.
The nm_connection_get_id() mock here returns "Wired connection 1" for both calls, which implies the wifiConn ID also matches the ethernet knownConnectionID. In activateKnownConnection(), ID matching happens before any type gating, so this setup can cause the loop to select the WiFi connection and break early, never exercising the ethernet filtering behavior the test name describes. Make the first ID (wifiConn) something non-matching (e.g., "MyWiFiNetwork") and the second (ethConn) "Wired connection 1".
| .WillOnce(::testing::Return("Wired connection 1")) // ethConn - match | |
| .WillOnce(::testing::Return("Wired connection 1")); // ethConn - for activation | |
| .WillOnce(::testing::Return("MyWiFiNetwork")) // wifiConn - non-matching ID | |
| .WillOnce(::testing::Return("Wired connection 1")); // ethConn - matching ID for activation |
| EXPECT_CALL(*p_libnmWrapsImplMock, nm_client_activate_connection_async(::testing::_, ::testing::_, ::testing::_, ::testing::_, ::testing::_, ::testing::_, ::testing::_)) | ||
| .WillOnce(::testing::Invoke([](NMClient* client, NMConnection* connection, NMDevice* device, const char* specific_object, GCancellable* cancellable, GAsyncReadyCallback callback, gpointer user_data) { | ||
| if (callback) { | ||
| GObject* source_object = G_OBJECT(client); | ||
| GAsyncResult* result = nullptr; | ||
| callback(source_object, result, user_data); | ||
| } | ||
| })); |
There was a problem hiding this comment.
This test doesn’t verify that nm_client_activate_connection_async() is invoked with the ethernet connection (ethConn). Without matching/asserting the connection argument, the test can pass even if the wrong connection is activated (especially given the current nm_connection_get_id() setup). Update the EXPECT_CALL to match ethConn (or assert inside the Invoke lambda) so the test actually enforces the intended filtering.
…r into topic/unit_test
…r into topic/unit_test
Reason for Change: Added the unit test case for this test scenario
Test Procedure: Run the unit test workflow for libnm
Priority: P1
Risks: Medium
Signed-off-by: Gururaaja ESRGururaja_ErodeSriranganRamlingham@comcast.com